Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hosted Zone Context Provider #425

Closed
wants to merge 1 commit into from
Closed

Hosted Zone Context Provider #425

wants to merge 1 commit into from

Conversation

moofish32
Copy link
Contributor

WIP - I am opening the PR a little early for some discussion.

Context Providers can close a big gap for legacy CFN customers because
we can now lookup values using the AWS SDK.

The design as is, seems a bit weird in that there is part of the
interface in @aws-cdk/cdk and the other part in aws-cdk. The second
part that doesn't feel quite right is the args[] array. I understand
to keep this private and in our control, but would something more like a
map of [string]: any make more sense?

If we are good with this path forward, I'll get some integration tests
and deal with the pagination aspect to the APIs.

By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.

@rix0rrr rix0rrr self-assigned this Jul 30, 2018
@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 31, 2018

The provider can also live in @aws-cdk/aws-route53, where it might make more sense. I agree that it does not make a lot of sense for @aws-cdk/core to know about Route53.

The fact that the "server" part of the implementation lives in aws-cdk is an implementation detail that's born out of pragmatism, but there's no reason that couldn't be moved somewhere else either. We can move those without impact to the user, so I'm not too worried (it's not a one-way door, in that way).

The second part that doesn't feel quite right is the args[] array.

You're right that this might as well have been a map. What we need from it is a consistent serialization to a string (in particular, with order), which an array satisfies quite easily. We could do something like "key1=value:key2=value:key3=value" for a map, if the keys are ordered. In fact, we can probably merge the two disparate arrays for scope and args together.

If you feel like making that change, you're very welcome to it. Otherwise feel free to make an issue for us.

public getZone(filter: HostedZoneFilter): string {
const scope = this.provider.accountRegionScope('HostedZoneProvider');
const args: string[] = [filter.name];
if (filter.privateZone) { args.push(`${filter.privateZone}`); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intended as a test against undefined or a test against true?

* Return the hosted zone meeting the filter
*/
public getZone(filter: HostedZoneFilter): string {
const scope = this.provider.accountRegionScope('HostedZoneProvider');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this intended to be "hosted-zone"?

/**
* Plugin to read arbitrary SSM parameter names
*/
export class HostedZoneContextProviderPlugin implements ContextProviderPlugin {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An instance of this needs to be added to availableContextProviders in cdk.ts as well.

const [account, region] = scope;
const domainName = args[0];
const privateZone: boolean = args[1] ? args[1] === 'true' : false;
const vpcId = args[2];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args[2] might not exist.

@eladb
Copy link
Contributor

eladb commented Aug 5, 2018

I wonder if the right API for this type of context provider is a static methods similar to HostedZone.import, so users can naturally find it:

const zone: HostedZoneRef = HostedZone.load(this, 'MyExistingHostedZone', { 
  /* filter */ 
});

It seems like this mechanism can be very relevant for certain resources (e.g. #506). Maybe we should think about a common pattern/idiom for supporting those in a discoverable and a clean way.

@moofish32
Copy link
Contributor Author

@eladb -- I think load or lookup makes a lot of sense for me. I think that filter is another flexible data structure. I don't fully understand @rix0rrr comment about ordering in the array being required yet. I need to go look at the code some more. I was hoping to have more of the duck typing, similar to golang passing around interface{} and casting.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 9, 2018

I don't fully understand @rix0rrr comment about ordering in the array being required

These are context values, and context values have a controlled update path. They're cached upon first lookup and then kept the same until the user consciously refreshes them, to make sure templates don't change out from under users without them expecting so.

That means that they need to be stored, and that means they need to be serialized--in practice, to a key-value object in cdk.json. The key is based on the "context query", which right now is the name of the of context provider and the stringified value of each of the arguments (combined in long string with : between them).

Maybe an example is simpler:

{
   "context": {
    "availability-zones:1234567:us-east-1": ["us-east-1a", "us-east-1b"],
    "ssm:1234567:us-east-1:/MyParam": "SomeValue"
  }
}

The only thing we need to guarantee is that every query that "means the same" (in effect, set of parameters) leads to the same key in the context object. With an array with concatenation, that's easy! Combine values and it's automatically true. For a dictionary, TYPICALLY that's not so easy since there is no well-defined order for the keys in the dictionary, and it COULD be that every iteration over the keys of a dictionary returns them in a different order. The only thing that means is we need to impose a consistent order on the keys ourselves, so that every serialization is the same.

(Now in practice, in JavaScript it so happens that it maintains insertion order, but for example you would also want the following to be true:

key({ foo: 'foo', bar: 'bar' }) == key({ bar: 'bar', foo: 'foo' })

Which is for example not true if we simply used JSON.stringify as the key function, or did a naive iteration over the keys.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 9, 2018

Oh and I'd prefer for the key to be a little aesthetically pleasing as well, which is why I suggested

bar=bar:foo=foo

as a map serialization.

@eladb
Copy link
Contributor

eladb commented Sep 27, 2018

@moofish32 any updates on this? Should we put it on hold for now? Do you need help getting this aligned with master after the indentation change?

@moofish32
Copy link
Contributor Author

@eladb I would like to come back to this in about a week. I would like to propose some changes to the ContextProvider framework to use some typed inputs. There were many things I didn't understand in this first attempt.

However, if the team is set on this pattern I will update and stay in the current flow. Do I have time to propose something in the next 2 weeks?

@moofish32
Copy link
Contributor Author

I don't think the indent will be a problem, I just changed my settings back to 2, this was only 4 project.

@moofish32
Copy link
Contributor Author

closing and moving to #823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants